Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

token 2022: add InitializeMember instruction from SPL Token Group interface #5629

Merged
merged 3 commits into from
Nov 29, 2023
Merged

token 2022: add InitializeMember instruction from SPL Token Group interface #5629

merged 3 commits into from
Nov 29, 2023
+629 −10

Conversation

buffalojoec
Copy link
Contributor

This PR adds the InitializeMember instruction to Token2022 - from the SPL Token Group interface.

This was referenced Oct 22, 2023
Merged
Merged
Closed
Merged
Merged
Closed
Merged
Merged
Merged
@buffalojoec Graphite App
Copy link
Contributor Author

buffalojoec commented Oct 22, 2023 •
edited
Loading

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@buffalojoec buffalojoec mentioned this pull request Oct 22, 2023
Merged
@buffalojoec buffalojoec force-pushed the 10-22-token_2022_add_GroupMemberPointer_extension branch from c28bc25 to e52e102 Compare October 22, 2023 12:25
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch from f16668e to b2f1959 Compare October 22, 2023 12:26
@buffalojoec buffalojoec mentioned this pull request Oct 26, 2023
Merged
@buffalojoec buffalojoec added the do-not-close Add this tag to exempt a PR / issue from being closed automatically label Nov 12, 2023
@buffalojoec buffalojoec mentioned this pull request Nov 14, 2023
Closed
@buffalojoec buffalojoec force-pushed the 10-22-token_2022_add_GroupMemberPointer_extension branch from e52e102 to 5f3942d Compare November 14, 2023 14:36
@buffalojoec buffalojoec mentioned this pull request Nov 14, 2023
Closed
@buffalojoec buffalojoec changed the base branch from 10-22-token_2022_add_GroupMemberPointer_extension to 11-13-token_2022_add_alloc_and_serialize_allow_repeating_ November 14, 2023 14:37
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch from b2f1959 to 12dfa57 Compare November 14, 2023 14:37
This was referenced Nov 14, 2023
Closed
Closed
@buffalojoec buffalojoec changed the base branch from 11-13-token_2022_add_alloc_and_serialize_allow_repeating_ to 10-22-token_2022_add_GroupMemberPointer_extension November 16, 2023 21:36
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch 2 times, most recently from 50f7ad7 to 5f75990 Compare November 16, 2023 21:47
@buffalojoec buffalojoec force-pushed the 10-22-token_2022_add_GroupMemberPointer_extension branch from bbd4c02 to c9ec232 Compare November 16, 2023 21:47
@buffalojoec buffalojoec marked this pull request as ready for review November 16, 2023 22:00
buffalojoec
buffalojoec commented Nov 16, 2023
View reviewed changes
Copy link
Contributor Author

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's ready for another look as well!

@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch from 9ab3658 to 2aa4fcc Compare November 17, 2023 09:12
@buffalojoec buffalojoec force-pushed the 10-22-token_2022_add_GroupMemberPointer_extension branch from d674818 to 9c4d472 Compare November 20, 2023 13:50
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch from 447f098 to 8d2e4ec Compare November 20, 2023 13:50
@buffalojoec buffalojoec force-pushed the 10-22-token_2022_add_GroupMemberPointer_extension branch from 9c4d472 to cb4de48 Compare November 20, 2023 15:46
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch from 8d2e4ec to 143792c Compare November 20, 2023 15:47
@buffalojoec buffalojoec force-pushed the 10-22-token_2022_add_GroupMemberPointer_extension branch from cb4de48 to 106a697 Compare November 21, 2023 10:52
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch 2 times, most recently from b3a3295 to 2a56a1d Compare November 21, 2023 13:00
@buffalojoec
Copy link
Contributor Author

buffalojoec commented Nov 22, 2023 •
edited
Loading

@joncinque I'm still debugging some weird behavior on this one. It seems like every once in a while, the transaction error for InsufficientFundsForRent throws on account at index 3 instead of 4.

Edit: I revisited this a bit, and it seems like the ordering of the writable_non_signer accounts are being shuffled. So, sometimes it goes

[3]: group mint
[4]: member mint

and sometimes

[3]: member mint
[4]: group mint

but haven't narrowed down why exactly.

@buffalojoec buffalojoec marked this pull request as draft November 22, 2023 15:35
joncinque
joncinque reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small points, this is looking good!

Regarding your question: the issue likely has to do with the ordering of the pubkeys in the transaction -- since you're generating random keypairs, sometimes one account will appear before another one in the BTreeMap of compiled account keys. If you want to test it for sure, you can check the ordering of the keypairs that you generate, or maybe simpler, just use keypairs from static arrays like [1; 32]

token/program-2022/src/extension/token_group/processor.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/token_group/processor.rs Outdated Show resolved Hide resolved
token/program-2022-test/tests/token_group_initialize_member.rs Outdated Show resolved Hide resolved
token/program-2022-test/tests/token_group_initialize_member.rs Outdated Show resolved Hide resolved
@buffalojoec buffalojoec force-pushed the 10-22-token_2022_add_GroupMemberPointer_extension branch from 106a697 to fbcb302 Compare November 28, 2023 01:07
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch 2 times, most recently from 18502c4 to 2d39f9c Compare November 28, 2023 02:05
@buffalojoec buffalojoec marked this pull request as ready for review November 28, 2023 02:05
@buffalojoec
Copy link
Contributor Author

Regarding your question: the issue likely has to do with the ordering of the pubkeys in the transaction -- since you're generating random keypairs, sometimes one account will appear before another one in the BTreeMap of compiled account keys.

Are we at all concerned with this behavior? In other words, is it worth looking into ensuring the order of accounts is first dictated by role and then secondarily dictated by the order in which it appears in an instruction? As I type it out, probably not, but thought I'd ask.

@joncinque
Copy link
Contributor

Are we at all concerned with this behavior? In other words, is it worth looking into ensuring the order of accounts is first dictated by role and then secondarily dictated by the order in which it appears in an instruction? As I type it out, probably not, but thought I'd ask.

The behavior is actually dictated in that way, and it's also sorted by pubkey. So if you have two writable accounts in a transaction, by definition the "lower" pubkey will appear first.

@buffalojoec
Copy link
Contributor Author

Are we at all concerned with this behavior? In other words, is it worth looking into ensuring the order of accounts is first dictated by role and then secondarily dictated by the order in which it appears in an instruction? As I type it out, probably not, but thought I'd ask.

The behavior is actually dictated in that way, and it's also sorted by pubkey. So if you have two writable accounts in a transaction, by definition the "lower" pubkey will appear first.

I think I'm specifically tunnel-visioning it around this use case I encountered. Even if I did attempt to change it to something else, it wouldn't make debugging transaction errors like InsufficientLamportsForRent any easier. So forget it!

Base automatically changed from 10-22-token_2022_add_GroupMemberPointer_extension to master November 28, 2023 15:44
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch from fd1b5de to 1c934b1 Compare November 28, 2023 16:18
joncinque
joncinque previously approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last comment, but then it's good to go!

club

token/program-2022-test/tests/token_group_initialize_member.rs Outdated Show resolved Hide resolved
@buffalojoec buffalojoec force-pushed the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch from 1c934b1 to c8b6093 Compare November 29, 2023 17:08
@buffalojoec buffalojoec mentioned this pull request Nov 29, 2023
Merged
@mergify mergify bot dismissed joncinque’s stale review November 29, 2023 17:08

Pull request has been modified.

@buffalojoec Graphite App
Copy link
Contributor Author

buffalojoec commented Nov 29, 2023 •
edited
Loading

Merge activity

@buffalojoec buffalojoec merged commit 3c82062 into master Nov 29, 2023
67 checks passed
@buffalojoec buffalojoec deleted the 10-18-token_2022_add_InitializeMember_instruction_from_SPL_Token_Group_interface branch November 29, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joncinque joncinque joncinque approved these changes

Assignees
No one assigned
Labels
do-not-close Add this tag to exempt a PR / issue from being closed automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@buffalojoec @joncinque